Skip to content

Don't use owner to determine if the auto-bailout should be used#4089

Merged
sebmarkbage merged 1 commit intofacebook:masterfrom
sebmarkbage:killowner
Aug 19, 2015
Merged

Don't use owner to determine if the auto-bailout should be used#4089
sebmarkbage merged 1 commit intofacebook:masterfrom
sebmarkbage:killowner

Conversation

@sebmarkbage
Copy link
Copy Markdown
Contributor

I didn't realize that we actually special cased this. This is an
unfortunate heuristic but it helps minimize the harm that this optimization
does.

Should we try to remove it?

See #4067 (comment)

@sebmarkbage
Copy link
Copy Markdown
Contributor Author

@spicyj, This could mean that deeply mutating a static element is actually a common pattern and we might break code by removing this special case. :/

@sophiebits
Copy link
Copy Markdown
Collaborator

I knew about this and thought we were planning to take this out after fixing prop mutations internally.

@sebmarkbage
Copy link
Copy Markdown
Contributor Author

Not having props mutation is no guarantee that this won't break code. So we might as well break it now, maybe?

@sophiebits
Copy link
Copy Markdown
Collaborator

Right, but it's harder ergonomically to deeply mutate an element that's created within render, no? I figured it's a little safer to wait but maybe I'm missing something. lgtm either way.

@sebmarkbage
Copy link
Copy Markdown
Contributor Author

This change doesn't affect elements created within render. It is easy to mutate objects that live outside of render. Then reuse those deep within an element but those created within render are often recreated.

Those created outside of render are as easy to mutate deeply as shallow. Even though they're more likely to be shallow and we might find them earlier because of it, it doesn't change the risk significantly.

On Jun 11, 2015, at 4:22 PM, Ben Alpert notifications@github.com wrote:

Right, but it's harder ergonomically to deeply mutate an element that's created within render, no? I figured it's a little safer to wait but maybe I'm missing something. lgtm either way.


Reply to this email directly or view it on GitHub.

@sophiebits sophiebits added this to the 0.14 milestone Aug 1, 2015
I didn't realize that we actually special cased this. This is an
unfortunate heuristic but it helps minimize the harm that this optimization
does.
sebmarkbage added a commit that referenced this pull request Aug 19, 2015
Don't use owner to determine if the auto-bailout should be used
@sebmarkbage sebmarkbage merged commit 4a92860 into facebook:master Aug 19, 2015
@sebmarkbage
Copy link
Copy Markdown
Contributor Author

Didn't run travis. #fingerscrossed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants